Support deleting whole partitions in Iceberg table by hantangwangd · Pull Request #21048 · prestodb/presto

hantangwangd · 2023-10-05T19:10:53Z

Description

We can use delete from <table> [where <predicate>]; to delete one or more whole partitions or all data in Iceberg table when some conditions are met:

The predicate could be wholly pushed down and exactly enforced.
The predicate Indicated one or more whole partitions in Iceberg table.

Motivation and Context

This PR allows deleting partition level data from Iceberg tables.

Test Plan

Make sure filter push down optimization do not affect existing test cases in IcebergDistributedTestBase
Newly added test case in IcebergDistributedTestBase.testDelete()

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

== RELEASE NOTES ==

General Changes
* The Iceberg connector now supports `DELETE FROM <table> [where <filter>]` statements to delete one or more entire partitions

ZacBlanco

My main concern is about semantics of delete with snapshots/partition evolution. It's not clear to me how exactly this is handled. Also, we should probably update the docs. Looks good otherwise though!!

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedTestBase.java

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java

ZacBlanco

Looks good! Mostly just small nits now. I think we should also update the docs to specify this is now supported and limited to partition-only and without specific snapshots

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergTableHandle.java

presto-iceberg/src/main/java/com/facebook/presto/iceberg/optimizer/IcebergPlanOptimizer.java

presto-main/src/main/java/com/facebook/presto/util/GraphvizPrinter.java

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java

github-actions · 2023-10-10T07:39:54Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 384aca5...6fcc0eb.

Notify	File(s)
@steveburnett	presto-docs/src/main/sphinx/connector/iceberg.rst

hantangwangd · 2023-10-10T08:40:56Z

Looks good! Mostly just small nits now. I think we should also update the docs to specify this is now supported and limited to partition-only and without specific snapshots

Fixes are done, and iceberg doc is updated. Please take a look.

ZacBlanco

Two minor grammatical suggestions on the docs, but @steveburnett might have better ones. Otherwise LGTM!

presto-docs/src/main/sphinx/connector/iceberg.rst

hantangwangd · 2023-10-10T14:39:34Z

Two minor grammatical suggestions on the docs, but @steveburnett might have better ones. Otherwise LGTM!

@ZacBlanco Thanks a lot, I will squash them into one commit later.

steveburnett

Nice work! I made a couple of suggestions for consistency and clarity.

presto-docs/src/main/sphinx/connector/iceberg.rst

steveburnett · 2023-10-10T15:15:29Z

presto-docs/src/main/sphinx/connector/iceberg.rst

Latin such as etc., e.g, i.e, should be avoided. For an example of a standard for this, see the GitLab recommended word list entry for etc.. To address this adapting @ZacBlanco's suggestion, consider something like this.

Suggested change

Do not support non-comparison operator for the filter columns, for example ADD, SUBTRACT, MODULUS, etc.

Filtered columns only support comparison operators, such as EQUALS, LESS THAN, or LESS THAN EQUALS.

presto-docs/src/main/sphinx/connector/iceberg.rst

hantangwangd · 2023-10-10T16:41:22Z

Nice work! I made a couple of suggestions for consistency and clarity.

@steveburnett Thanks a lot. I have update the docs following your suggestion, and squash them into one commit. Please take a look.

steveburnett

LGTM! (docs)

Updated my local branch just now and built the docs locally to pick up and review the latest changes as they will be seen in production, everything looks great.

ChunxuTang

@hantangwangd Thanks for your work! This is a nice feature that would benefit the connector users.
Just left some minor comments.

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergTableHandle.java

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java

presto-main/src/main/java/com/facebook/presto/util/GraphvizPrinter.java

ChunxuTang · 2023-10-10T17:16:46Z

@hantangwangd BTW, do you want to add a release note?

hantangwangd · 2023-10-10T18:36:34Z

@hantangwangd BTW, do you want to add a release note?

@ChunxuTang Sure, release note have been added.

We can use 'delete from <table> [where <predicate>]' to delete whole partitions or all data in Iceberg table. The predicate should satisfy some requirements: - The columns appeared in predicate should all be identity partitioned columns, that is, use their original value as partition value. - The columns appeared in predicate should not be wrapped in a non-comparison operator(for example ADD, SUBTRACT, MODULUS etc.) Co-authored-by: Zac Blanco <zac@ibm.com> Co-authored-by: Steve Burnett <burnett@pobox.com>

ChunxuTang

LGTM. @hantangwangd Thanks for the work again.

presto-main/src/main/java/com/facebook/presto/util/GraphvizPrinter.java

ChunxuTang · 2023-10-11T21:06:42Z

Re-running the failed test.

hantangwangd · 2023-10-11T23:13:30Z

LGTM. @hantangwangd Thanks for the work again.

@ChunxuTang It's my pleasure.

hantangwangd requested a review from a team as a code owner October 5, 2023 19:10

hantangwangd requested a review from presto-oss October 5, 2023 19:10

tdcmeehan added the iceberg Apache Iceberg related label Oct 5, 2023

tdcmeehan requested review from ChunxuTang and ZacBlanco October 5, 2023 19:25

hantangwangd force-pushed the master branch from e9192ec to 287d8cd Compare October 6, 2023 02:36

ZacBlanco requested changes Oct 6, 2023

View reviewed changes

tdcmeehan self-assigned this Oct 6, 2023

hantangwangd force-pushed the master branch from 287d8cd to 71d84f7 Compare October 8, 2023 03:50

ZacBlanco requested changes Oct 9, 2023

View reviewed changes

hantangwangd force-pushed the master branch from 71d84f7 to 9c882c0 Compare October 10, 2023 07:38

ZacBlanco approved these changes Oct 10, 2023

View reviewed changes

presto-docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved

steveburnett requested changes Oct 10, 2023

View reviewed changes

hantangwangd force-pushed the master branch from 7ecb6ca to 7d59158 Compare October 10, 2023 16:37

steveburnett approved these changes Oct 10, 2023

View reviewed changes

ChunxuTang reviewed Oct 10, 2023

View reviewed changes

hantangwangd force-pushed the master branch from 7d59158 to 43be36b Compare October 10, 2023 18:31

hantangwangd force-pushed the master branch from 43be36b to 6fcc0eb Compare October 11, 2023 00:54

ChunxuTang approved these changes Oct 11, 2023

View reviewed changes

presto-main/src/main/java/com/facebook/presto/util/GraphvizPrinter.java Outdated Show resolved Hide resolved

hantangwangd merged commit bb433ab into prestodb:master Oct 11, 2023

hantangwangd mentioned this pull request Nov 3, 2023

Add test case for deletion in Iceberg table with partition evolution #21298

Merged

6 tasks

wanglinsong mentioned this pull request Dec 8, 2023

Add release notes for 0.285 #21500

Closed

26 tasks

	Do not support non-comparison operator for the filter columns, for example ADD, SUBTRACT, MODULUS, etc.
	Filtered columns only support comparison operators, such as EQUALS, LESS THAN, or LESS THAN EQUALS.

Conversation

hantangwangd commented Oct 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Test Plan

Contributor checklist

Release Notes

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hantangwangd commented Oct 10, 2023

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hantangwangd commented Oct 10, 2023

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

steveburnett Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hantangwangd commented Oct 10, 2023

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

ChunxuTang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChunxuTang commented Oct 10, 2023

Uh oh!

hantangwangd commented Oct 10, 2023

Uh oh!

ChunxuTang left a comment • edited by hantangwangd Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ChunxuTang commented Oct 11, 2023

Uh oh!

hantangwangd commented Oct 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hantangwangd commented Oct 5, 2023 •

edited

Loading

github-actions bot commented Oct 10, 2023 •

edited

Loading

ChunxuTang left a comment •

edited by hantangwangd

Loading